Skip to content

Fixes #39134 - Add bulk modal state helper to core#10893

Merged
ofedoren merged 1 commit intotheforeman:developfrom
ATIX-AG:move_foremanmodal_fix_to_foreman
Mar 6, 2026
Merged

Fixes #39134 - Add bulk modal state helper to core#10893
ofedoren merged 1 commit intotheforeman:developfrom
ATIX-AG:move_foremanmodal_fix_to_foreman

Conversation

@nadjaheitmann
Copy link
Copy Markdown
Contributor

@nadjaheitmann nadjaheitmann commented Mar 4, 2026

The fix was originally implemented in Katello but can also be used for other plugins, so it makes more sense to have it in Foreman core:

Katello/katello#11626

@github-actions github-actions Bot added the UI label Mar 4, 2026
@nadjaheitmann nadjaheitmann mentioned this pull request Mar 4, 2026
Merged
@nadjaheitmann nadjaheitmann force-pushed the move_foremanmodal_fix_to_foreman branch from b73f573 to e2edca5 Compare March 4, 2026 07:54
@nadjaheitmann nadjaheitmann force-pushed the move_foremanmodal_fix_to_foreman branch 2 times, most recently from ebccbac to a76295c Compare March 4, 2026 09:40
@nadjaheitmann
Copy link
Copy Markdown
Contributor Author

nadjaheitmann commented Mar 4, 2026

Not sure if anything else needs to be added to make it available in 'foremanReact' directory for plugins.

Lukshio
Lukshio suggested changes Mar 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@Lukshio Lukshio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for bringing this up.

I think that better place for this helper will be inside in file BulkModalStateHelper.js foreman/webpack/assets/javascripts/react_app/common

Also I would like to add some inline comments and description of this PR to define usage of this helper. It should be used only in situations where useState hook is not available, ex. when modal is mounted by global fill or cases like that.

@nadjaheitmann nadjaheitmann force-pushed the move_foremanmodal_fix_to_foreman branch from a76295c to 4872f14 Compare March 5, 2026 12:52
@nadjaheitmann
Copy link
Copy Markdown
Contributor Author

nadjaheitmann commented Mar 5, 2026 •
edited
Loading

Thank you for the feedback!

I think that better place for this helper will be inside in file BulkModalStateHelper.js foreman/webpack/assets/javascripts/react_app/common

Done.

Also I would like to add some inline comments and description of this PR to define usage of this helper. It should be used only in situations where useState hook is not available, ex. when modal is mounted by global fill or cases like that.

I tried to add a descriptive comment but I am not sure if I grasped the technical depth correctly. @Lukshio can you please double check and correct where necessary.

@Lukshio
Copy link
Copy Markdown
Contributor

Lukshio commented Mar 5, 2026

Thank you, looks good now, test failures are not related.

@ofedoren
Copy link
Copy Markdown
Member

ofedoren commented Mar 5, 2026

It might be my OCD or misunderstanding, but:

The PR refs a ticket that was release and is even in a different project, I'd advise to create a new one and link them, especially if we plan to refactor Katello to import this instead of using what's there already, since it'll require a new ticket which logically follows this one. Otherwise it'll be a bunch of refs and a bit chaotic.

Also, the fix states replace, but it doesn't replace anything, it just adds something new that is not used anywhere yet.

@nadjaheitmann
Copy link
Copy Markdown
Contributor Author

nadjaheitmann commented Mar 5, 2026

The PR refs a ticket that was release and is even in a different project, I'd advise to create a new one and link them, especially if we plan to refactor Katello to import this instead of using what's there already, since it'll require a new ticket which logically follows this one. Otherwise it'll be a bunch of refs and a bit chaotic.

Good point. I'll do that.

Also, the fix states replace, but it doesn't replace anything, it just adds something new that is not used anywhere yet.

Well, it does replace it in Katello. You are right, I should have put more attention to the commit message. It is confusing the way it is. Thanks @ofedoren

@Lukshio @nadjaheitmann
Fixes #39134 - Add bulk modal state helper to core
951e90f 
The fix was originally implemented in Katello but can also be used for
other plugins, so it makes more sense to have it in Foreman core:

Katello/katello#11626
@nadjaheitmann nadjaheitmann force-pushed the move_foremanmodal_fix_to_foreman branch from 4872f14 to 951e90f Compare March 5, 2026 15:19
@nadjaheitmann nadjaheitmann changed the title Refs #39047 - replace useForemanModal in BulkActions Fixes #39134 - Add bulk modal state helper to core Mar 5, 2026
@nadjaheitmann
Copy link
Copy Markdown
Contributor Author

nadjaheitmann commented Mar 5, 2026

I have added a new issue, linked it to the Katello issue and rephrased the commit message.

ofedoren
ofedoren approved these changes Mar 6, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @nadjaheitmann and @Lukshio !

@ofedoren ofedoren merged commit 40938e4 into theforeman:develop Mar 6, 2026
26 checks passed
@nadjaheitmann nadjaheitmann deleted the move_foremanmodal_fix_to_foreman branch March 6, 2026 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ofedoren ofedoren ofedoren approved these changes

+1 more reviewer

@Lukshio Lukshio Lukshio requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

UI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nadjaheitmann @Lukshio @ofedoren